Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v5.0.0-alpha.24 #24614

Merged
merged 21 commits into from
Jan 26, 2021
Merged

v5.0.0-alpha.24 #24614

merged 21 commits into from
Jan 26, 2021

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jan 25, 2021

Rendered changelog
Rendered release workflow instructions

Starting with this release, changelogs should be based on yarn release:changelog. For tagging, please use yarn release:tag. If you've done releases before, please check out the new instructions.

For all:

  • decide on highlights
  • sort contributors (would've expected appearance in changelog but that doesn't seem to be the case)
    Sorted by alphanumeric asc and moved to the end

For reviewer:

  • changelog grouping
  • changelog script usage
  • updated release script

For me:

  • generate changelog in dev CI
  • resolve TODO items
  • update package versions
  • missing explicit dependency

@eps1lon eps1lon added the on hold There is a blocker, we need to wait label Jan 25, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Jan 25, 2021

Details of bundle changes

Generated by 🚫 dangerJS against 4d48ff1

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@eps1lon
Copy link
Member Author

eps1lon commented Jan 25, 2021

One thing we need to clarify better: How to tag.

alpha.23 was associated with a commit that is not part of this remote (af6e722).

  • [ ] I'm investigating if we can rewrite this without breaking every working tree. Let's not risk it. We can guard against this in the future.

I'll probably wrap this in a script that checks if the commit is part of the correct remote.

git commit -am "v5.0.0-alpha.18"
git tag v5.0.0-alpha.18
```
1. `yarn relase:version`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current behavior seems broken to me. It prompts to update every single package. Might be due to lightweight tag usage. We'll see next time if that is the case.

If lerna expects to not bump any packages I would expect an explicit option for that. Right now I can only trick it by entering the same version but that's tedious.

Copy link
Member

@oliviertassinari oliviertassinari Jan 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lerna is configured to treat the version of the packages individually. @material-ui/types needed a different one.

In this mode and from what I could gather, Lerna needs one tag per managed version in order to only bump the correct packages in the next release. This was flooding the "Tags" tab view on GitHub. The current situation is the result of a tradeoff: bump everything (cons) in exchange for packages with multiple versions and a single tag (pros).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not talking about tagging but updating version in the package.json. I don't understand why lerna thinks e.g. @material-ui/envinfo was changed.

I suspect it's either caused by lightweight tags or the last tag pointing to a commit that doesn't exist on the mui-org/material-ui remote. Hopefully it's not just bad DX.

@mnajdova
Copy link
Member

One thing we need to clarify better: How to tag.

alpha.23 was associated with a commit that is not part of this remote (af6e722).

I'll probably wrap this in a script that checks if the commit is part of the correct remote.

Should we create these PRs on upstream then (mui-org)?

@eps1lon
Copy link
Member Author

eps1lon commented Jan 25, 2021

Should we create these PRs on upstream then (mui-org)?

This should be fixed in the new workflow. You won't run git tag on branches anymore. Just need to update the instructions to push it to mui-org/material-ui not the default remote. I don't know which commit you tagged. It doesn't seem to be the one you pushed to mui-org/material-ui

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@eps1lon
Copy link
Member Author

eps1lon commented Jan 25, 2021

Will merge the latest state of next tomorrow after lunch (1400 CET), update the changelog, merge PR and release.

@eps1lon
Copy link
Member Author

eps1lon commented Jan 26, 2021

Update: Items are now numbered by their date ascending. This ensures that the numbers stay the same over time. This makes it easier to review changelog changes over time (e.g. when new items are added).

@eps1lon eps1lon changed the title DO NOT MERGE Release v5.0.0-alpha.24 Release v5.0.0-alpha.24 Jan 26, 2021
@eps1lon eps1lon removed the on hold There is a blocker, we need to wait label Jan 26, 2021
@eps1lon eps1lon merged commit 15108a5 into mui:next Jan 26, 2021
@eps1lon eps1lon deleted the scripts/changelog branch January 26, 2021 14:03
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 26, 2021

Update: Items are now numbered by their date ascending. This ensures that the numbers stay the same over time. This makes it easier to review changelog changes over time (e.g. when new items are added).

@eps1lon Why optimize for when each change was merged? I mean what problem does it solve for developers? I was under the assumption that all the changes are released at the same time from a developer's perspective.

So far, items have been ordered alphabetically because it makes it easier, for developers, to review changes specific to a component (grouping + predictable order). Developers don't use all the components, they might only care about a subset, e.g. Accordion.

Capture d’écran 2021-01-26 à 15 27 09

@eps1lon
Copy link
Member Author

eps1lon commented Jan 26, 2021

Why optimize for when each change was merged?

Release are ordered in time so we might as well keep the order inside a release

So far, items have been ordered alphabetically because it makes it easier, for developers, to review changes specific to a component, e.g. Accordion.

They were never ordered alphabetically though.

it makes it easier, for developers, to review changes specific to a component, e.g. Accordion.

You can still do that with searching. Assuming that these are grouped is somewhat dangerous because we never explained how each components affects each other. If they assume every change affecting Accordion is grouped under [Accordion] then they ignore changes to e.g. ButtonBase. And I'm not even going into the whole "what is grouped under [Accordion]?" topic. That is currently even more confusing. Seems to me that we just removed a footgun.

Please review more thoroughly in the future. I requested the review, even pointed to the items, yet you approved. If you don't review a PR, please don't approve it. That gives a false sense of security.

@eps1lon
Copy link
Member Author

eps1lon commented Jan 26, 2021

And please stop doing screenshots, thanks. Either link to the text or copy it so that system defaults of text can be applied.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 26, 2021

You can still do that with searching

True, it's fair. My intent was to explain why it was sorted alphabetically so far, and to get a sense of why we were changing it for developers. The first motivation shared seemed to be maintainers-focused, which could/should be secondary. While I don't understand the motivation/problem we solve, no objections to giving this a try.

Thanks for sharing the update with a dedicated component, I didn't notice the change in the first passe. I think that the image was important to contextualize the use case here, to get a feeling of the end-to-end DX (from the update developers received by email when they subscribe).

What do you mean by they were never ordered alphabetically? The usage of .sort() was meant to achieve this outcome. Do you mean that it should have used toLocaleCompare?


Well done with the release 🙏

@oliviertassinari oliviertassinari added the release We are shipping :D label Dec 21, 2021
@oliviertassinari oliviertassinari changed the title Release v5.0.0-alpha.24 v5.0.0-alpha.24 Dec 21, 2021
@michaldudak michaldudak mentioned this pull request Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release We are shipping :D
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants